Conversation
…oll isolation, Playwright tests Agent-Logs-Url: https://github.com/Devn913/SQL_Chess/sessions/eb41956a-9637-482d-a220-c795c0d325b3 Co-authored-by: Devn913 <56478595+Devn913@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
Improves the right-side SQL panel UX by making the layout responsive across viewports, enlarging the query editor, and isolating scrolling to avoid body scroll issues (especially on mobile), with automated Playwright coverage and CI enforcement.
Changes:
- Updated CSS responsive rules for chess/SQL panels, including scroll isolation and viewport-specific sizing.
- Increased SQL textarea rows and enabled vertical resizing with min/max height constraints.
- Added Playwright responsive/layout tests plus a CI workflow job to run them.
Reviewed changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
css/style.css |
Responsive layout + scroll isolation adjustments; SQL panel sizing and textarea sizing rules. |
index.html |
Increases SQL textarea rows from 3 → 6. |
tests/responsive.spec.js |
Adds Playwright tests validating layout, scrolling behavior, and key SQL-panel interactions. |
playwright.config.js |
Introduces Playwright config and multi-viewport projects. |
package.json |
Adds Playwright devDependency and test scripts. |
.github/workflows/deploy.yml |
Adds Playwright test job and makes deploy depend on validate + test. |
test-results/.last-run.json |
Adds Playwright run artifact file (likely unintended to commit). |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| { | ||
| "status": "passed", | ||
| "failedTests": [] | ||
| } No newline at end of file |
There was a problem hiding this comment.
test-results/.last-run.json is a generated Playwright artifact and will change across runs; committing it will create noisy diffs and potential merge conflicts. It should be removed from version control and the test-results/ directory should be added to .gitignore (similar to playwright-report/).
| { | |
| "status": "passed", | |
| "failedTests": [] | |
| } |
| test('chess panel and SQL panel are side-by-side on wide screens', async ({ page, viewport }) => { | ||
| if (!viewport || viewport.width < 1024) test.skip(); | ||
| await openAndStartGame(page); |
There was a problem hiding this comment.
viewport is not a built-in Playwright test fixture, so destructuring async ({ page, viewport }) leaves viewport undefined. As written, the test.skip() guards will always trigger (or behave incorrectly), meaning these viewport-dependent layout assertions won’t actually run. Use page.viewportSize() or read the configured viewport via testInfo.project.use.viewport (or define a custom fixture) and base the skip conditions on that value.
| /** | ||
| * Helper: open the page, dismiss the setup modal by playing as guest, | ||
| * then return the page for further assertions. | ||
| */ | ||
| async function openAndStartGame(page) { | ||
| await page.goto(PAGE_URL); | ||
| // Dismiss setup modal | ||
| await page.click('#btnPlayAsGuest'); | ||
| } |
There was a problem hiding this comment.
The JSDoc for openAndStartGame says it "return[s] the page", but the function doesn’t return anything. Either return page (so callers can use the return value) or update the comment to match the current behavior.
| use: { | ||
| /* Serve index.html directly via file:// so no server is needed */ | ||
| baseURL: 'file://' + path.resolve(__dirname, 'index.html'), | ||
| headless: true, | ||
| screenshot: 'only-on-failure', | ||
| video: 'off', | ||
| }, |
There was a problem hiding this comment.
use.baseURL is configured here, but the tests use their own PAGE_URL and call page.goto(PAGE_URL) directly, so baseURL is currently unused. Consider either switching tests to page.goto('/') (and drop the per-test PAGE_URL), or removing baseURL from the config to avoid having two sources of truth for the test entrypoint.
| node-version: '20' | ||
|
|
||
| - name: Install dependencies | ||
| run: npm ci |
There was a problem hiding this comment.
This job uses npm ci, but the repo ignores package-lock.json (see .gitignore:3) and there doesn’t appear to be a lockfile committed. npm ci will fail without a lockfile. Either commit a lockfile (and stop ignoring it) or switch this step to npm install (optionally with a pinned Node + caching) so CI can install Playwright reliably.
| run: npm ci | |
| run: npm install |
The right SQL panel had a cramped 3-row textarea (cutting off the default sample query), no scroll isolation (body could scroll on mobile), and poor layout across viewports.
CSS / Layout
SQL panel:
flex: 0 0 32%(was 30%),min-width: 300px;hidden-paneloverride documented with commentScroll isolation:
sql-contentis the sole scroll region (overflow-y: auto,min-height: 0); body and outer containers remainoverflow: hiddenat all breakpointsResponsive breakpoints (7 distinct queries):
flex-growsQuery Textarea
rows3 → 6;min-height: 7.5rem;max-height: 20rem;resize: vertical(wasnone)Tests
bodyoverflow hidden, onlysql-contentscrolls), side-by-side vs. stacked layout, no horizontal overflow, board squareness, SQL toggle, block generation on moves, run-SQL execution and error displaytestjob (Playwright + Chromium); deploy now requires bothvalidateandtestto pass